Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: ContentHoverWidget respects Theia styles #14836

Conversation

kittaakos
Copy link
Contributor

@kittaakos kittaakos commented Feb 4, 2025

What it does

Customize the default ContentHoverWidget behavior to ensure it uses Theia styles when calculating available space for the hover widget.

VS Code uses 30 pixels for the top height and 24 pixels for the bottom height, but Theia uses 32 pixels for the top and 22 for the bottom.

Closes #14826

This PR fixes the available space calculation for the content hover widget in the monaco editor. As described in #14826, the Theia top and bottom height (menu + status bar) are different in VS Code. In some cases, the popup either was on the menu or was hidden under the Theia toolbar. This PR fixes both cases.

How to test

Please, as shown in the issue, try to make the editor hover either overlay the menu or hide below the toolbar when active. Let me know if you still find glitches.

In-action:

theia14826_part1.mov
theia14826_part2.mov

Follow-ups

Breaking changes

  • This PR introduces breaking changes and requires careful review. If yes, the breaking changes section in the changelog has been updated.

Attribution

Review checklist

Reminder for reviewers

Customize the default `ContentHoverWidget` behavior to ensure it uses
Theia styles when calculating available space for the hover widget.

VS Code uses 30 pixel for top height, and 24 pixels for bottom height,
but Theia uses 32 pixel for the top and 22 for the bottom.

Closes eclipse-theia#14826
Copy link
Contributor

@martin-fleck-at martin-fleck-at left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The change looks good to me overall and seems to work well, thank you Akos! I'm just a bit worried about the hard-coded numbers and the difference between handling the top diff vs the bottom diff. Maybe you could elaborate on that a bit more.

packages/monaco/src/browser/monaco-init.ts Outdated Show resolved Hide resolved
@kittaakos
Copy link
Contributor Author

In-action with c198028:

Screen.Recording.2025-02-05.at.15.47.31.mov
Screen.Recording.2025-02-05.at.15.51.17.mov

Copy link
Contributor

@martin-fleck-at martin-fleck-at left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes look great to me now, thank you for the quick turnaround! I just noticed a minor thing in the new copyright headers, they all say: Copyright (C) 2025 and others.. Do you maybe want to add your company or personal name to that?

@kittaakos
Copy link
Contributor Author

Do you maybe want to add your company or personal name to that?

If it's optional to leave it blank for now, I would choose to do so; I’m not quite sure what should or could go there at this time. Thank you!

Copy link
Contributor

@martin-fleck-at martin-fleck-at left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@martin-fleck-at martin-fleck-at merged commit 58244d6 into eclipse-theia:master Feb 5, 2025
11 checks passed
@github-actions github-actions bot added this to the 1.59.0 milestone Feb 5, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

monaco hover popup hidden by the toolbar
2 participants